Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch Soroban tests to always use closeLedger to apply txs. #4455

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

dmkozh
Copy link
Contributor

@dmkozh dmkozh commented Sep 4, 2024

Description

Resolves #4451

Switch Soroban tests to always use closeLedger to apply txs.

This is a necessary change for the BucketListDB support as it doesn't materialize changes on LTX commit. I've also changed a single test ('basic contract invocation') to only use in-memory LTX in order to cover lower-level logic.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@dmkozh dmkozh force-pushed the soroban_tests_bldb branch 2 times, most recently from 253ffd5 to 37b5303 Compare September 5, 2024 16:05
@dmkozh dmkozh marked this pull request as ready for review September 5, 2024 16:05
@dmkozh dmkozh changed the title Soroban tests BLDB WIP for CI Switch Soroban tests to always use closeLedger to apply txs. Sep 5, 2024
@SirTyson SirTyson self-requested a review September 6, 2024 21:42
Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this change, looks like you had to wade through off by one hell for this one...

Overall LGTM and is very helpful for my BucketList test changes! A few small nit picks and on unfortunate interface inconsistency with TransactionTestFrame.

src/transactions/test/SorobanTxTestUtils.cpp Outdated Show resolved Hide resolved
src/transactions/test/SorobanTxTestUtils.h Outdated Show resolved Hide resolved
src/transactions/test/SorobanTxTestUtils.cpp Show resolved Hide resolved
src/transactions/test/InvokeHostFunctionTests.cpp Outdated Show resolved Hide resolved
src/transactions/test/InvokeHostFunctionTests.cpp Outdated Show resolved Hide resolved
@SirTyson SirTyson added this pull request to the merge queue Sep 10, 2024
@SirTyson SirTyson removed this pull request from the merge queue due to a manual request Sep 10, 2024
This is a necessary change for the BucketListDB support.  I've also changed a single test ('basic contract invocation') to only use in-memory LTX in order to cover lower-level logic.
@dmkozh dmkozh added this pull request to the merge queue Sep 10, 2024
Merged via the queue into stellar:master with commit 55bc7b4 Sep 10, 2024
14 checks passed
@dmkozh dmkozh deleted the soroban_tests_bldb branch September 10, 2024 21:09
github-merge-queue bot pushed a commit that referenced this pull request Sep 14, 2024
# Description

Resolves #4433

This switches the default testing backend to BucketListDB. While some
test still disable BucketListDB and commit directly via `LedgerTxn` to
test lower level subsystem, such as operation application, the majority
of tests and all end-to-end tests no use BucketListDB. This is rebased
on top of #4455.

# Checklist
- [x] Reviewed the
[contributing](https://github.com/stellar/stellar-core/blob/master/CONTRIBUTING.md#submitting-changes)
document
- [x] Rebased on top of master (no merge commits)
- [x] Ran `clang-format` v8.0.0 (via `make format` or the Visual Studio
extension)
- [x] Compiles
- [x] Ran all tests
- [ ] If change impacts performance, include supporting evidence per the
[performance
document](https://github.com/stellar/stellar-core/blob/master/performance-eval/performance-eval.md)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Soroban tests need to work with closeLedger
2 participants